feat(core): Export a reusable function to add tracing headers#20076
feat(core): Export a reusable function to add tracing headers#20076
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Other
Bug Fixes 🐛Core
Other
Internal Changes 🔧Core
Deps
Other
🤖 This preview updates automatically when you update the PR. |
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/core/src/fetch.ts
Outdated
| type PolymorphicRequestHeaders = | ||
| | Record<string, string | undefined> | ||
| | Array<[string, string]> | ||
| | Iterable<Iterable<string>> |
There was a problem hiding this comment.
Accepted Iterable type lacks runtime handling in function
Low Severity
The new Iterable<Iterable<string>> variant was added to PolymorphicRequestHeaders, which is used as the type for fetchOptionsObj.headers. However, the function body only handles Headers instances (via isHeaders), arrays (via Array.isArray), and plain objects (the else branch). A non-array, non-Headers iterable (e.g. a Map) would silently fall into the else branch, where in operator checks and object spread would not correctly process the iterable's entries, producing malformed headers. Since this function is now exported for cross-package use, the type promises broader acceptance than the implementation supports.
Additional Locations (1)
There was a problem hiding this comment.
This type is needed for Cloudflare headers. I've added a test to also check for that case.
JPeer264
force-pushed
the
jp/export-trace-headers
branch
from
d9322e7 to
1622527
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: feat PR lacks integration or E2E test
- Added a Node.js integration test that demonstrates cross-package usage of getTracingHeadersForFetchRequest, validating that the function correctly generates tracing headers when imported from @sentry/core.
Or push these changes by commenting:
@cursor push 7c341385d0
Preview (7c341385d0)
diff --git a/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/scenario.ts
new file mode 100644
--- /dev/null
+++ b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/scenario.ts
@@ -1,0 +1,30 @@
+import * as Sentry from '@sentry/node';
+import { getTracingHeadersForFetchRequest } from '@sentry/core';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+ dsn: 'https://public@dsn.ingest.sentry.io/1337',
+ tracesSampleRate: 1.0,
+ transport: loggingTransport,
+});
+
+Sentry.startSpan({ name: 'test-span' }, () => {
+ const headers = getTracingHeadersForFetchRequest('https://example.com/api', {});
+
+ if (!headers || typeof headers !== 'object' || Array.isArray(headers)) {
+ throw new Error('getTracingHeadersForFetchRequest returned invalid headers');
+ }
+
+ const sentryTrace = 'sentry-trace' in headers ? headers['sentry-trace'] : undefined;
+ const baggage = 'baggage' in headers ? headers.baggage : undefined;
+
+ if (!sentryTrace || typeof sentryTrace !== 'string' || !sentryTrace.match(/^[0-9a-f]{32}-[0-9a-f]{16}-[01]$/)) {
+ throw new Error('sentry-trace header is invalid or missing');
+ }
+
+ if (!baggage || typeof baggage !== 'string' || !baggage.includes('sentry-')) {
+ throw new Error('baggage header is invalid or missing');
+ }
+
+ Sentry.captureMessage('test message');
+});
diff --git a/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/test.ts b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/test.ts
new file mode 100644
--- /dev/null
+++ b/dev-packages/node-integration-tests/suites/public-api/getTracingHeadersForFetchRequest/basic-usage/test.ts
@@ -1,0 +1,17 @@
+import { afterAll, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+afterAll(() => {
+ cleanupChildProcesses();
+});
+
+test('should generate tracing headers for fetch request', async () => {
+ await createRunner(__dirname, 'scenario.ts')
+ .expect({
+ event: {
+ message: 'test message',
+ },
+ })
+ .start()
+ .completed();
+});This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
JPeer264
requested review from
chargome and
s1gr1d
and removed request for
Lms24 and
nicohrubec
|
q: IIC you want to use this in Cloudflare afterwards? I think we have serveral conventions for internal functions but lately we've been making use of an |
|
Yes for now only for Cloudflare, but in theory this can be then used for other instrumentations then. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8100e43. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
JPeer264
force-pushed
the
jp/export-trace-headers
branch
from
8100e43 to
4e235e3
Compare
packages/core/src/fetch.ts
Outdated
| * 3. Previous baggage header has sentry baggage values -> do nothing (might have been added manually by users) | ||
| * @internal Exported for cross-package instrumentation (for example Cloudflare Workers fetcher bindings) | ||
| * and unit tests | ||
| * @hidden |
There was a problem hiding this comment.
| * @hidden |
very logaf-L
packages/core/src/fetch.ts
Outdated
|
|
||
| return headers.every( | ||
| (item): item is [string, string] => | ||
| Array.isArray(item) && item.length === 2 && typeof item[0] === 'string' && typeof item[1] === 'string', |
There was a problem hiding this comment.
l/m: I think this added condition is too narrow. A header value doesn't have to be a string. I just tried a request like
fetch('https://github.com', {headers: [['x-test', 2]]})in the chrome dev console and chrome sent the request with the added header value. So looks like this gets coerced to a string if it's not one in the first place. Might be relevant IRL for cache ttl or body size headers, if users add them manually 🤔
We can probably just omit the typeof check for the 2nd value, wdyt?
There was a problem hiding this comment.
True, but then the types were wrong before, I'll update them to also match other values. Dropping the second value would make sense in this case.
There was a problem hiding this comment.
I just made the second value to be unknown. It is still worth to mention that undici types (and types from lib.dom.d.ts had the same types. But making it more general overall is safer.
There was a problem hiding this comment.
huh interesting, thanks for checking the types! Tbh this makes it logaf-L for me then but let's better be safe than sorry.
| | Record<string, unknown> | ||
| | Array<[string, unknown]> | ||
| | Iterable<Iterable<unknown>> |
There was a problem hiding this comment.
Bug: The function _INTERNAL_getTracingHeadersForFetchRequest doesn't handle Iterable header types like Map, causing user-provided headers to be silently discarded when passed.
Severity: HIGH
Suggested Fix
Add a new branch to _INTERNAL_getTracingHeadersForFetchRequest to handle Iterable<Iterable<unknown>> types. This branch should convert the iterable into a Headers object or a plain object before merging it with the Sentry tracing headers. For example, new Headers(originalHeaders) can be used to correctly process the iterable.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/fetch.ts#L22-L24
Potential issue: The `PolymorphicRequestHeaders` type was expanded to include
`Iterable<Iterable<unknown>>` to support environments like Cloudflare Workers. However,
the implementation of `_INTERNAL_getTracingHeadersForFetchRequest` was not updated to
handle this case. When an iterable that is not an array (like a `Map`) is passed as
headers, it falls into an `else` block that incorrectly assumes it's a plain object.
Using the spread operator (`...`) on a `Map` to create a new object results in an empty
object, causing all original headers to be silently dropped from the request.
3 participants
This PR is an extraction of #19991
It basically exports
getTracingHeadersForFetchRequest, which was previously only exported for testing, but offers a great functionality if you want to add tracing headers to a request. I renamed it asaddTracingHeadersToFetchRequestsounded a little misleading, as it didn't really add headers to the request, as it returned the extracted headers from the request (or init, if there are any).Open question
I added
@hiddenand@internalto it, not sure if this is an approach we follow. I'm ok to remove it from the jsdoc